Query history: Add new VariantAnalysisHistoryItem type#1590
Query history: Add new VariantAnalysisHistoryItem type#1590shati-patel merged 7 commits intomainfrom
VariantAnalysisHistoryItem type#1590Conversation
aeisenberg
left a comment
aeisenberg
left a comment
There was a problem hiding this comment.
I commented on a few locations where we should be handling an unexpected type. Also, we should probably be bumping the serialization version number.
| case 'remote': | ||
| queryPath = finalSingleItem.remoteQuery.queryFilePath; | ||
| break; | ||
| case 'variant-analysis': |
There was a problem hiding this comment.
You should probably add a default case.
| } else { | ||
| } else if (item.t === 'remote') { | ||
| await this.removeRemoteQuery(item); | ||
| } else if (item.t === 'variant-analysis') { |
There was a problem hiding this comment.
Add an else block to handle unknown types.
| return item.remoteQuery.queryText; | ||
| case 'variant-analysis': | ||
| return 'TODO'; | ||
| } |
| // queries aged out. | ||
| return asyncFilter(parsedQueries, async (q) => { | ||
| if (q.t === 'remote') { | ||
| if (q.t === 'remote' || q.t === 'variant-analysis') { |
There was a problem hiding this comment.
On line 18, there is a version property that gets added. We probably want to bump this to version 2. This would mean that in the rare case someone downgrades from a version of the extension with the variant-analysis type to one without, they won't have errors when loading query history (it will mean that they lose their query history, but I think that is a reasonable tradeoff).
shati-patel
force-pushed
the
shati-patel/variant-analysis-history-item
branch
from
5971292 to
127a23e
Compare
|
Thanks for the review, @aeisenberg! Addressed your comments 🤩 And thank you for pointing out the query serialization versions—I didn't know about those 👀 Would you mind double-checking that the version bump looks okay? 🙇🏽♀️ (see 127a23e) |
aeisenberg
left a comment
aeisenberg
left a comment
There was a problem hiding this comment.
Thanks. Looks good. A few more things I noticed.
| const data = await fs.readFile(fsPath, 'utf8'); | ||
| const obj = JSON.parse(data); | ||
| if (obj.version !== 1) { | ||
| if (obj.version !== 1 && obj.version !== 2) { |
There was a problem hiding this comment.
This is fine. In the future we could change it to [1, 2].includes(obj. version). This will make it easier to extend.
| const obj = JSON.parse(data); | ||
| if (obj.version !== 1) { | ||
| if (obj.version !== 1 && obj.version !== 2) { | ||
| void showAndLogErrorMessage(`Unsupported query history format: v${obj.version}. `); |
There was a problem hiding this comment.
Reading this message now, and it won't make much sense to users. Perhaps we can change it? Maybe something like this:
| void showAndLogErrorMessage(`Unsupported query history format: v${obj.version}. `); | |
| void showAndLogErrorMessage(`Can't parse query history. Unsupported query history format v${obj.version}.`); |
| const filteredQueries = queries.filter(q => q.t === 'local' ? q.completedQuery !== undefined : true); | ||
| const data = JSON.stringify({ | ||
| version: 1, | ||
| version: 2, |
There was a problem hiding this comment.
Can you add a comment here documenting what has changed between v1 and v2?
3 participants
Adds a new
VariantAnalysisHistoryItemtype toQueryHistoryInfo, so that the query history view will be able to display variant analysis items in future.This PR adds the type itself and some placeholder "TODO"s to get the query history code to type check. Actually implementing the methods is tracked in a follow-up issue.
See internal linked issue for details!
Checklist
N/A - internal only 🦞
ready-for-doc-reviewlabel there.